Refactor FXIOS-14344 [Swift 6 migration] Fixing warnings in unit tests #32
Conversation
WalkthroughThe pull request systematically converts XCTest lifecycle methods across 20+ test files from synchronous to asynchronous throwing variants, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift`:
- Around line 28-33: Move the cleanup (nil assignments and mock reset) to run
before calling super. In the tearDown() implementation, first set
diffableDataSource = nil, collectionView = nil and call
DependencyHelperMock().reset(), then call try await super.tearDown() so cleanup
always executes even if super throws; update the tearDown() method accordingly.
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swift`:
- Around line 26-28: The testRequestAuthorization() currently calls
notificationManager.requestAuthorization and uses assert() inside the completion
handler which may not run before the test exits and is stripped in Release;
convert this to an XCTest expectation or an async test so the test waits for the
completion, and replace assert(...) with XCTest assertions (e.g.
XCTAssertTrue/granted and XCTAssertTrue(center?.requestAuthorizationWasCalled ??
false) or XCTAssertNotNil for center) executed on the main actor; specifically
update the code that calls notificationManager.requestAuthorization { [center]
(granted, error) in ... } to fulfill an expectation (or await an async API)
inside that closure and then waitForExpectations or mark the test async so
assertions inside the completion are reliably executed.
🧹 Nitpick comments (3)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewControllerWebViewDelegates.swift (1)
23-45: Consider markingMockNavigationActionasfinalfor consistency.
MockFrameInfo,MockURLAuthenticationChallengeSender, andMockFileManagerare all markedfinal, butMockNavigationActionis not. For consistency and the same Swift 6 migration benefits, consider addingfinalhere as well.♻️ Proposed fix
// MARK: - MockNavigationAction -class MockNavigationAction: WKNavigationAction { +final class MockNavigationAction: WKNavigationAction { private var type: WKNavigationType?firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockUserNotificationCenter.swift (1)
9-9: Confirm Sendable safety before using@unchecked.This mock has mutable state with no synchronization;
@unchecked Sendablebypasses compiler checks and can mask data races if it’s used across tasks. If tests only touch this on the main actor, prefer actor isolation (e.g.,@MainActor) to get safety guarantees; otherwise add explicit synchronization.♻️ Example (main-actor isolation)
- class MockUserNotificationCenter: UserNotificationCenterProtocol, `@unchecked` Sendable { + `@MainActor` + class MockUserNotificationCenter: UserNotificationCenterProtocol {firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/RatingPromptManagerTests.swift (1)
36-46: Movesuper.tearDown()to the end of the method.XCTest best practice dictates that
setUp()callssuperfirst, whiletearDown()callssuperlast. This ensures your test state is torn down before the superclass cleanup runs, keeping the lifecycle symmetrical.♻️ Suggested reorder
override func tearDown() async throws { - try await super.tearDown() prefs.clearAll() subject.reset() prefs = nil logger = nil urlOpenerSpy = nil userDefaults = nil crashTracker = nil subject = nil + try await super.tearDown() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/FxAWebViewModelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/RatingPromptManagerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkViewModelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Library/HistoryPanelTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuConfigurationUtilityTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyCoordinatorTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewControllerWebViewDelegates.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockUserNotificationCenter.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/NativeErrorPage/NativeErrorPageViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift
🧰 Additional context used
🧬 Code graph analysis (8)
firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swift (4)
firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swift (2)
setUp(14-18)tearDown(20-23)firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (2)
setUp(16-20)tearDown(22-25)firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyCoordinatorTests.swift (2)
setUp(15-20)tearDown(22-25)firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/Launch/LaunchCoordinatorTests.swift (2)
setUp(18-25)tearDown(27-33)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
firefox-ios/Storage/SQL/SQLiteQueue.swift (1)
getQueuedTabs(24-34)
firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swift (3)
firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (2)
setUp(16-20)tearDown(22-25)firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swift (2)
setUp(15-20)tearDown(22-27)firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swift (2)
setUp(15-20)tearDown(22-27)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swift (5)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift (2)
setUp(16-26)tearDown(28-33)firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swift (2)
setUp(14-18)tearDown(20-23)firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (2)
setUp(16-20)tearDown(22-25)firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swift (2)
setUp(15-20)tearDown(22-27)firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swift (2)
setUp(15-20)tearDown(22-27)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkViewModelTests.swift (4)
firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (2)
setUp(16-20)tearDown(22-25)firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swift (2)
setUp(15-22)tearDown(24-29)firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swift (2)
setUp(14-18)tearDown(20-23)firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditFolderViewModelTests.swift (2)
setUp(32-38)tearDown(40-46)
firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuConfigurationUtilityTests.swift (6)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift (2)
setUp(16-26)tearDown(28-33)firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swift (2)
setUp(15-22)tearDown(24-29)firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swift (2)
setUp(15-20)tearDown(22-27)firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/BrowserCoordinatorTests.swift (2)
setUp(28-45)tearDown(47-60)BrowserKit/Tests/MenuKitTests/MenuMainViewTests.swift (2)
setUp(12-16)tearDown(18-21)BrowserKit/Tests/MenuKitTests/MenuTableViewHelperTests.swift (2)
setUp(13-17)tearDown(19-23)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swift (5)
firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swift (2)
setUp(14-18)tearDown(20-23)firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (2)
setUp(16-20)tearDown(22-25)firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swift (2)
setUp(15-22)tearDown(24-29)firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swift (2)
setUp(14-18)tearDown(20-23)firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swift (4)
setUp(15-20)setupStore(714-717)tearDown(22-27)resetStore(719-721)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swift (3)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockUserNotificationCenter.swift (1)
requestAuthorization(22-26)firefox-ios/Client/NotificationManager.swift (2)
requestAuthorization(48-54)requestAuthorization(67-73)firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockNotificationManager.swift (3)
requestAuthorization(14-17)requestAuthorization(19-20)requestAuthorization(22-24)
🪛 SwiftLint (0.57.0)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swift
[Warning] 14-14: Implicitly unwrapped optionals should be avoided when possible
(implicitly_unwrapped_optional)
🔇 Additional comments (28)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewControllerWebViewDelegates.swift (1)
48-54: LGTM! Addingfinalaligns with Swift 6 migration.The
finalmodifier is a good addition for mock classes that won't be subclassed—it enables static dispatch and helps the compiler with optimizations.firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/RatingPromptManagerTests.swift (1)
22-23: AsyncsetUp()conversion looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkViewModelTests.swift (1)
33-45: LGTM!The async throwing lifecycle methods follow the correct pattern:
setUp: callstry await super.setUp()first before initializing test fixturestearDown: cleans up test fixtures first, then callstry await super.tearDown()lastThis is consistent with the established pattern in other test files in this PR (e.g.,
IntroViewModelTests.swift).firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewModelTests.swift (1)
16-25: LGTM!The async throwing lifecycle methods are correctly implemented with proper ordering of super calls. The
@MainActorannotation on the class is appropriate for testing view model logic that may interact with UI components.firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
93-98: LGTM!The completion handler signature now correctly includes
@MainActor@sendable@escaping``, which matches the production implementation inSQLiteQueue.swift. This ensures proper Swift 6 concurrency compliance for closures that cross actor boundaries.firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/HistoryPanelTests.swift (1)
14-25: LGTM!The async throwing lifecycle methods follow the correct pattern with proper super call ordering. The
@MainActorannotation is appropriate for testingHistoryPanel, which is a UI component.firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuCoordinatorTests.swift (1)
14-23: LGTM!The async throwing lifecycle methods are correctly implemented with proper ordering of super calls. This is consistent with other coordinator tests in the codebase (e.g.,
LaunchCoordinatorTests,MicrosurveyCoordinatorTests).firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyMiddlewareTests.swift (1)
15-27: LGTM!The async throwing lifecycle methods are correctly implemented with proper ordering of super calls. This is consistent with other middleware tests in the codebase (e.g.,
MainMenuMiddlewareTests).firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuMiddlewareTests.swift (2)
15-20: Async setup migration looks good.
22-27: Async teardown migration looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuConfigurationUtilityTests.swift (2)
17-21: Async setup migration looks good.
23-27: Async teardown migration looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/FxAWebViewModelTests.swift (2)
14-21: Async setup migration looks good.
23-27: Async teardown migration looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift (2)
11-12: Annotation change looks good.
16-26: Async setup migration looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyCoordinatorTests.swift (2)
15-20: Async setup migration looks good.
22-25: Async teardown migration looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/OnboardingTests/IntroViewControllerTests.swift (2)
14-18: Async setup migration looks good.
20-23: Async teardown migration looks good.firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/SyncContentSettingsViewControllerTests.swift (2)
15-22: AsyncsetUp()migration looks correct.
try await super.setUp()is properly chained for XCTest async lifecycle.
24-28: AsynctearDown()migration looks correct.Cleanup plus
try await super.tearDown()aligns with the async XCTest lifecycle.firefox-ios/firefox-ios-tests/Tests/ClientTests/MainMenu/MainMenuViewControllerTests.swift (1)
10-11: Main-actor annotation fits this UI test.Running the test case on the main actor is appropriate for UIKit usage.
firefox-ios/firefox-ios-tests/Tests/ClientTests/Microsurvey/MicrosurveyViewControllerTests.swift (1)
10-11: Main-actor annotation fits this UI test.This keeps UI operations on the main actor.
firefox-ios/firefox-ios-tests/Tests/ClientTests/NativeErrorPage/NativeErrorPageViewControllerTests.swift (1)
10-11: Main-actor annotation fits this UI test.Keeps test execution aligned with UIKit’s main-thread requirements.
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swift (3)
8-9: Main-actor annotation fits this UI test.Keeps the test on the main actor for UIKit interactions.
16-27: Async lifecycle migration looks correct.
try await super.setUp()/try await super.tearDown()are wired correctly.
14-14: File not found in repository.The file
firefox-ios/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/EditBookmarkDataSourceTests.swiftdoes not exist in the current repository. The specified line and IUO warning cannot be verified. This review comment references a file that is either no longer present, located at a different path, or was not included in this repository state.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| override func tearDown() async throws { | ||
| try await super.tearDown() | ||
| diffableDataSource = nil | ||
| collectionView = nil | ||
| DependencyHelperMock().reset() | ||
| super.tearDown() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
XCTestCase async tearDown exception handling best practices
💡 Result:
Best practices for XCTestCase async tearDown + exception/error handling
1) In tearDown() async throws, only do work that is actually awaited
If you schedule work with DispatchQueue.async/asyncAfter, XCTest won’t wait for it (because those APIs aren’t async), so cleanup may never run. Prefer await-based APIs (or wrap callback-based APIs with continuations). (stackoverflow.com)
override func tearDown() async throws {
try await apiClient.deleteTestData()
await super.tearDown()
}2) Prefer teardown blocks (addTeardownBlock) for per-test cleanup (especially when partially-initialized)
Register cleanup right after you allocate/create a resource, so it gets cleaned up even if the test fails midway.
Notes from practice/documentation summaries:
- teardown blocks run after the test method and before the
tearDownmethods - the block can
awaitandthrow - you can register teardown blocks from the test or
setUp, but not fromtearDown(useyourloaf.com)
func testSomething() async throws {
let id = try await apiClient.createThing()
addTeardownBlock {
try await apiClient.deleteThing(id: id)
}
// ... assertions ...
}3) Decide whether teardown errors should fail the test or be best-effort
- If you want cleanup failures to fail the test, let the error propagate by using
tearDownWithError/tearDown() async throws. (Throwing from these methods is treated as a test failure.) (useyourloaf.com) - If cleanup is best-effort (common for “try to delete remote test data”), catch and record, or just swallow—otherwise you can end up with many “failed tests” caused only by cleanup.
4) Avoid continueAfterFailure = false with async tests / async setUp / async tearDown
continueAfterFailure = false is implemented via Objective‑C exceptions; exceptions through Swift async frames are problematic/undefined, and newer Xcode versions changed behavior to avoid reliability issues. Practically: keep continueAfterFailure at the default true, and use throws/early-return patterns to stop the test. (gist.github.com)
5) Don’t start “detached” tasks in teardown unless you intentionally don’t care if they finish
If you do Task { ... } in teardown and don’t await it, the test can end before it completes, and the task may be cancelled or never observed. Prefer structured concurrency (await the work in teardown / teardown block). (stackoverflow.com)
Citations:
- 1: https://stackoverflow.com/questions/75108550/xctest-async-code-doesnt-get-executed-in-teardown-async-throws-method?utm_source=openai
- 2: https://useyourloaf.com/blog/unit-test-setup-and-teardown/?utm_source=openai
- 3: https://useyourloaf.com/blog/xctest-error-handling-improvements/?utm_source=openai
- 4: https://gist.github.com/steipete/84a5952c22e1ff9b6fe274ab079e3a95?utm_source=openai
- 5: https://stackoverflow.com/questions/75108550/xctest-async-code-doesnt-get-executed-in-teardown-async-throws-method?utm_source=openai
🌐 Web query:
Swift XCTest tearDown super call order cleanup
💡 Result:
In XCTest, the conventional (and safest) pattern is:
setUp…: callsuperfirst, then do your per-test setup.tearDown…: do your cleanup first, then callsuperlast. This matches Apple’s long-standing example (tearDownends with[super tearDown]). [1]
Why this order matters / what runs when:
- Any
addTeardownBlock { … }blocks run after the test method ends and before the test case’s teardown methods. Multiple teardown blocks run LIFO (last registered, first executed). [2] - For a given test, XCTest runs setup/teardown hooks in a defined sequence (showing the common overloads):
- Setup:
setUp() async throws→setUpWithError()→setUp() - Teardown:
tearDown()→tearDownWithError()→tearDown() async throws(and teardown blocks occur before these). [3]
- Setup:
Do you need to call super?
- For a direct subclass of
XCTestCase, the base implementations are effectively “template methods” and are often empty, so omittingsuperusually doesn’t change behavior today—but callingsuperkeeps things correct if you later introduce a shared test base class or a framework superclass adds behavior. [4]
Typical Swift shape
override func setUp() throws {
try super.setUp()
// create SUT, fixtures
}
override func tearDown() throws {
// nil out SUT, close files, cancel tasks, etc.
try super.tearDown()
}Sources: Apple archived testing guide example [1], teardown-block ordering notes [2], lifecycle ordering summary [3], discussion of why super calls are optional-but-wise [4].
[1] https://developer.apple.com/library/archive/documentation/DeveloperTools/Conceptual/testing_with_xcode/chapters/04-writing_tests.html
[2] https://useyourloaf.com/blog/unit-test-setup-and-teardown/
[3] https://useyourloaf.com/blog/unit-test-setup-and-teardown/
[4] https://stackoverflow.com/questions/53448538/xcode-10-and-super-teardown
Move cleanup before super.tearDown() to ensure it runs even if super throws.
XCTest's conventional pattern places cleanup before the super call in tearDown methods. If super.tearDown() throws an exception, the subsequent cleanup (nil assignments and DependencyHelperMock().reset()) won't execute, leaking state into later tests.
♻️ Proposed fix
override func tearDown() async throws {
- try await super.tearDown()
diffableDataSource = nil
collectionView = nil
DependencyHelperMock().reset()
+ try await super.tearDown()
}🤖 Prompt for AI Agents
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/StoriesFeed/StoriesFeedDiffableDataSourceTests.swift`
around lines 28 - 33, Move the cleanup (nil assignments and mock reset) to run
before calling super. In the tearDown() implementation, first set
diffableDataSource = nil, collectionView = nil and call
DependencyHelperMock().reset(), then call try await super.tearDown() so cleanup
always executes even if super throws; update the tearDown() method accordingly.
| notificationManager.requestAuthorization { [center] (granted, error) in | ||
| assert(granted, "Authorization should be granted") | ||
| assert(center?.requestAuthorizationWasCalled ?? false, "requestAuthorization should be called") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 5301
🌐 Web query:
Swift 6 XCTest assertions @sendable completion handlers best practices
💡 Result:
Best practices (Swift 6 + strict concurrency) for XCTest assertions with @Sendable completion handlers
1) Prefer async tests + await fulfillment(of:timeout:) over blocking waits
Blocking APIs like wait(for:timeout:) / waitForExpectations(timeout:) can deadlock when mixed with async work; Apple introduced the concurrency-safe fulfillment(of:timeout:enforceOrder:) specifically for this scenario. Use it in async tests. [1], [2]
func testFetch() async throws {
let exp = expectation(description: "callback")
api.fetch { exp.fulfill() }
await fulfillment(of: [exp], timeout: 1.0)
}2) Don’t put XCTest assertions in callbacks that may run concurrently unless you control isolation
If the completion can run on any executor/thread, treat it as concurrent:
- Make the completion
@Sendable(or accept asendingclosure—see#4). - Only capture
Sendablestate (or make captured stateSendable/immutable), or hop to an actor before touching isolated/mutable state. This mirrors the general Swift 6 rule that@Sendableclosures can’t safely capture non-Sendablevalues. [3]
api.fetch { `@Sendable` result in
Task { `@MainActor` in
XCTAssertEqual(result.value, 42)
}
}3) For “UI-style” completions that are guaranteed to run on the main thread, model that explicitly
If a callback is logically “main-actor-only”, prefer typing it as @MainActor (and in practice you may still see @Sendable involved depending on bridging/heuristics). One pragmatic pattern is @Sendable @MainActor`` for the closure type and then call it from a main-actor context. [4]
typealias Completion = `@Sendable` `@MainActor` () -> Void4) If you own an API that takes a completion handler, consider sending instead of @Sendable
Swift Evolution added sending to reduce caller friction vs requiring @Sendable everywhere (especially for “good old completion blocks”). That said, there have been Swift 6-era rough edges when passing sending closures from actor-isolated contexts (notably @MainActor). [5], [6]
func doWork(completion: sending `@escaping` () -> Void) { ... }5) Be aware XCTest’s async bridging can itself introduce @Sendable completion handlers
XCTest discovers async tests via an Objective-C thunk that takes a @Sendable completion handler; changes in actor isolation/default isolation can affect how XCTest sees/runs your tests. If you hit odd isolation/compiler issues, check your test target’s default actor isolation and where you apply @MainActor. [7]
Sources
[1] Swift Forums: deadlock + “use fulfillment(of:) instead” release note quote (Xcode 14.3+) — https://forums.swift.org/t/mixing-async-await-code-with-waitforexpectations-in-tests-causes-hang/62773
[2] Apple docs: XCTestCase.fulfillment(of:timeout:enforceOrder:) — https://developer.apple.com/documentation/xctest/xctestcase/fulfillment%28of%3Atimeout%3Aenforceorder%3A%29
[3] Stack Overflow: URLSession completion is @Sendable and triggers Swift 6 sendability errors — https://stackoverflow.com/questions/79205410/resolving-concurrency-sendable-errors
[4] Jesse Squires: @Sendable @MainActor`` completion pattern; notes Swift 6 improvements via SE-0434 — https://www.jessesquires.com/blog/2024/06/05/swift-concurrency-non-sendable-closures/
[5] Swift Forums: SE-0430 acceptance announcement (sending) — https://forums.swift.org/t/accepted-with-modifications-se-0430-second-review-sendable-parameter-and-result-values/71850
[6] Swift Forums: “friends don’t let friends declare good old completion blocks as `@Sendable`. They should be `sending`” + `@MainActor` caveat — https://forums.swift.org/t/relaxing-sendable-constraint-to-sending-closures-creates-undesired-warnings-errors-in-swift-6-language-mode/72367
[7] Swift Forums: XCTest async test thunk uses `@Sendable` completion handler; actor-isolation pitfalls — https://forums.swift.org/t/xctestcase-compiler-error-with-swift-6-2-default-actor-isolation-mainactor/83418
Test doesn't wait for completion handler to execute, making assertions invisible.
The testRequestAuthorization() method returns immediately after calling the completion-based API. The assertions inside the handler may never run or fail silently, resulting in a false-positive test. Additionally, assert() is stripped in Release builds and won't surface as XCTest failures.
Convert to an async test with an expectation to ensure the completion handler is awaited and assertions run reliably on the main actor:
Recommended fix
- func testRequestAuthorization() {
+ func testRequestAuthorization() async {
+ let exp = expectation(description: "requestAuthorization")
notificationManager.requestAuthorization { [center] (granted, error) in
- assert(granted, "Authorization should be granted")
- assert(center?.requestAuthorizationWasCalled ?? false, "requestAuthorization should be called")
+ Task { `@MainActor` in
+ XCTAssertTrue(granted, "Authorization should be granted")
+ XCTAssertTrue(center?.requestAuthorizationWasCalled ?? false, "requestAuthorization should be called")
+ exp.fulfill()
+ }
}
+ await fulfillment(of: [exp], timeout: 1)
}🤖 Prompt for AI Agents
In
`@firefox-ios/firefox-ios-tests/Tests/ClientTests/Helpers/NotificationManagerTests.swift`
around lines 26 - 28, The testRequestAuthorization() currently calls
notificationManager.requestAuthorization and uses assert() inside the completion
handler which may not run before the test exits and is stripped in Release;
convert this to an XCTest expectation or an async test so the test waits for the
completion, and replace assert(...) with XCTest assertions (e.g.
XCTAssertTrue/granted and XCTAssertTrue(center?.requestAuthorizationWasCalled ??
false) or XCTAssertNotNil for center) executed on the main actor; specifically
update the code that calls notificationManager.requestAuthorization { [center]
(granted, error) in ... } to fulfill an expectation (or await an async API)
inside that closure and then waitForExpectations or mark the test async so
assertions inside the completion are reliably executed.
Benchmark PR from agentic-review-benchmarks#6
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.